test: improve test quality in access_log_test.go with testify patterns#11748
Merged
test: improve test quality in access_log_test.go with testify patterns#11748
Conversation
- Add testify imports (assert and require) - Convert all manual error checks to require.NoError with helpful messages - Convert all manual value comparisons to assert.Equal with descriptive messages - Add t.Run() subtests to TestAnalyzeAccessLogsDirectory for better test organization - Update TestExtractDomainFromURL to use t.Run() for each test case - Add comprehensive TestParseSquidLogLine with 5 test cases (valid/invalid scenarios) - Add comprehensive TestAddMetrics with 3 test cases (aggregation scenarios) - All tests now follow specs/testing.md patterns Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Use assert.Len instead of assert.Equal for slice length comparisons - Use require.Error instead of assert.Error for error assertions in conditional blocks - All linter validations now pass Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Improve test quality in access_log_test.go
test: improve test quality in access_log_test.go with testify patterns
Jan 25, 2026
This was referenced Jan 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Converts
pkg/cli/access_log_test.gofrom manual error checking to testify assertions, adds missing test coverage, and applies table-driven test patterns witht.Run()subtests.Testify Migration
if err != nil { t.Fatalf() }→require.NoError(t, err, "msg")if got != want { t.Errorf() }→assert.Equal(t, expected, actual, "msg")assert.Lenfor slice lengths,require.Errorin conditionalsTest Organization
t.Run()subtests toTestAnalyzeAccessLogsDirectory("multiple access logs in subdirectory", "no access logs - returns nil")TestExtractDomainFromURLto uset.Run()per test caseNew Test Coverage
TestParseSquidLogLine: 5 cases covering valid/invalid squid log parsing (valid lines, insufficient fields, empty input)TestAddMetrics: 3 cases for metrics aggregation (valid addition, zero values, empty base)Before/After
Test count: 4 → 6 functions with organized subtests. Follows
specs/testing.mdconventions.Original prompt
This section details on the original issue you should resolve
<issue_title>[testify-expert] Improve Test Quality: pkg/cli/access_log_test.go</issue_title>
<issue_description>### Overview
The test file
pkg/cli/access_log_test.gohas been selected for quality improvement by the Testify Uber Super Expert. This file contains 4 test functions (182 lines) testing access log parsing functionality. While the tests are functional, they rely heavily on manual error checking rather than testify assertions, lack descriptive test names for edge cases, and have opportunities for table-driven test patterns.Current State
pkg/cli/access_log_test.gopkg/cli/access_log.goTest Quality Analysis
Strengths ✅
testutil.TempDir(t, "test-*")for proper test isolation with temporary directoriesAreas for Improvement 🎯
1. Testify Assertions
Current Issues:
if err != nil { t.Fatalf() }if got != want { t.Errorf() }assertorrequirepackagesRecommended Changes:
Why this matters: Testify provides:
specs/testing.mdconventionsFiles to update: All 4 test functions need conversion
2. Table-Driven Tests
Current Issues:
TestExtractDomainFromURL(lines 164-182) uses table-driven tests ✅ BUT doesn't uset.Run()for subtestsRecommended Changes:
Additional Opportunity:
TestAccessLogParsingandTestMultipleAccessLogAnalysistest similar patterns (allowed/blocked counting) and could benefit from table-driven tests for different log content scenarios.3. Test Coverage Gaps
Missing Tests:
Based on the source file, these exported functions lack dedicated tests:
AddMetrics(lines 41-47) - Method for aggregating domain analysis metricsparseSquidLogLine(lines 136-154) - Low-level log line parsing with error casesformatDomainWithEcosystem(line 193) - Domain formatting helperPriority Functions to Test:
parseSquidLogLine- High PriorityAddMetrics- Medium PriorityRecommended Test Cases: